core(lantern): move metrics to computed artifacts #4766

patrickhulce · 2018-03-14T23:32:04Z

splitting up big part of #4676 for separate discussion, this is a straight up refactor with no functional changes yet.

the bulk of predictive-perf audit has been split up into separate computed artifacts, they share a common base class (computed/metrics/metric.js) that DRYs up the simulation

if all this looks mostly good I'll add test file for each one individually

patrickhulce · 2018-03-14T23:33:33Z

lighthouse-core/gather/computed/metrics/metric.js

+   * @param {Object} artifacts
+   * @return {Promise<MetricResult>}
+   */
+  async computeMetricWithGraphs(data, artifacts, extras) {


openly searching for a better name and possible flow than extras, its needed for TTI to be able to pass in the FMP results for Math.max, alternative is just duplicating this entire computeMetricWithGraphs function for TTI

patrickhulce · 2018-03-19T20:17:31Z

anyone interested in giving this some review ❤️ ? :D

paulirish

generally fine with the guts of each metric in a computed artifact.

paulirish · 2018-03-19T20:23:55Z

lighthouse-core/gather/computed/metrics/consistently-interactive.js

@@ -0,0 +1,100 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


paulirish · 2018-03-20T00:00:52Z

lighthouse-core/gather/computed/metrics/metric.js

+    const traceOfTab = await artifacts.requestTraceOfTab(trace);
+    const networkAnalysis = await artifacts.requestNetworkAnalysis(devtoolsLog);
+
+    const optimisticGraph = this.getOptimisticGraph(graph, traceOfTab, extras);


afaict these extras arent used within this methods?

yeah just in getEstimateFromSimulation I'll remoeve

paulirish · 2018-03-20T00:08:55Z

lighthouse-core/gather/computed/metrics/metric.js

+    const optimisticEstimate = new Simulator(optimisticGraph, options).simulate();
+    const pessimisticEstimate = new Simulator(pessimisticGraph, options).simulate();
+
+    optimisticEstimate.timeInMs = this.getEstimateFromSimulation(


we're redefining the timeInMs? Feels a little weird. What does the timeInMs from .simulate represent?

yeah, from .simulate it's a shortcut for Math.max(...nodeTiming.map(timing => timing.endTime))

we want to return our optimistic/pessimistic estimates in ms along with the node timing that led to that computation, open to other suggestions on how to structure the return value

paulirish · 2018-03-20T00:09:54Z

lighthouse-core/runner.js

-    require('fs').readdirSync(__dirname + '/gather/computed').forEach(function(filename) {
+    const fileList = [
+      ...fs.readdirSync(path.join(__dirname, './gather/computed')),
+      ...fs.readdirSync(path.join(__dirname, './gather/computed/metrics')).map(f => `metrics/${f}`),


can you check if gulpfile also needs a bump?

this effectively is bumping the gulpfile from what I could tell

lighthouse/lighthouse-extension/gulpfile.js

Lines 35 to 36 in 32fd61f

const gatherers = LighthouseRunner.getGathererList()

.map(f => '../lighthouse-core/gather/gatherers/' + f.replace(/\.js$/, ''));

brendankenny

I started leaving specific comments but it seems like you were looking for an overall 👍 first :)

This is looking good to me as a refactor of the current state of things. These make sense as computed artifacts.

For naming, I made a comment below that we may want to classify these more as lantern-specific metrics. We may have other simulation approaches than optimistic/pessimistic regression, so we probably don't want metric as the going name for just lantern-style simulated metrics.

I'm also somewhat concerned by code like the growing beginning of createAuditResult :)

const records = [];
graph.traverse(node => node.record && records.push(node.record));
const simulatorOptions = NetworkAnalysis.computeRTTAndServerResponseTime(records);
delete simulatorOptions.rtt;
Object.assign(simulatorOptions, {cpuTaskMultiplier: 1, layoutTaskMultiplier: 1});
const simulator = new LoadSimulator(graph, simulatorOptions);

some complexity is inherent here (and some will be fixed with those TODOs), but it would be great to do a pass in the near future focused on readability (and possibility writability) of any APIs we use outside the computed artifacts

brendankenny · 2018-03-19T23:25:29Z

lighthouse-core/gather/computed/metrics/consistently-interactive.js

@@ -0,0 +1,100 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


brendankenny · 2018-03-19T23:26:09Z

lighthouse-core/gather/computed/metrics/consistently-interactive.js

+ */
+'use strict';
+
+const MetricArtifact = require('./metric');


since these are new files, can we turn on type checking for them?

brendankenny · 2018-03-19T23:39:47Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -364,3 +364,8 @@ module.exports = Simulator;
 * @property {number} [layoutTaskMultiplier]
 */

+/**
+ * @typedef SimulationResult


since these aren't exported I don't believe these will be visible in other files. Will need to live under LH...maybe a new sub-module under LH?

It will be structurally type checked across files, though, if you'd rather just define compatible typedefs in each place

I discovered they're referenceable when you require in the file as I was getting the simulator stuff type checked, that seems like a decent way to prevent LH externs from ballooning with the more local typedefs, wdyt?

const Simulator = require('../../simulator') /** * @param {Simulator.SimulationResult} result */

neeeeeeeevermind, that just worked last time as a coincidence because I export a property of that type with the same name on the class object 😞

gah, I swear I checked this with Audit and it didn't work for jsdoc typedefs. Maybe it was fixed in 2.8? I can't remember if I tested before or after we updated. Or it was just user error :)

But since it works, agree that that's good for this kind of typedef

brendankenny · 2018-03-19T23:43:24Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

+    graph.traverse(node => node.record && records.push(node.record));
+    const simulatorOptions = NetworkAnalysis.computeRTTAndServerResponseTime(records);
+    // TODO: use rtt/throughput from config.settings instead of defaults
+    delete simulatorOptions.rtt;


set to undefined?

unfortunately that won't have the same effect, gotta love JS key existence vs. undefined biting us left and right 😆

we should probably keep an eye on performance then, but hopefully properties on simulatorOptions aren't the bottleneck anyways... :)

🤣 🤣 🤣

brendankenny · 2018-03-19T23:49:05Z

lighthouse-core/gather/computed/metrics/metric.js

+const Simulator = require('../../../lib/dependency-graph/simulator/simulator');
+const WebInspector = require('../../../lib/web-inspector');
+
+class MetricArtifact extends ComputedArtifact {


SimulatedMetric or LanternMetric, maybe?

LanternMetric for now 👍

patrickhulce · 2018-03-20T17:26:06Z

For naming, I made a comment below that we may want to classify these more as lantern-specific metrics. We may have other simulation approaches than optimistic/pessimistic regression, so we probably don't want metric as the going name for just lantern-style simulated metrics.

Sure, not sure if you got a chance to take a look at #4676 but the idea was that the single file would encapsulate the proper metric value and do the context.throttlingType === 'lantern' ? this.getLanternMetric() : this.getTraceMetric for you so it's not duplicated in all audits, keeping lantern computed-artifact separate composable subclass sg though 👍

some complexity is inherent here (and some will be fixed with those TODOs), but it would be great to do a pass in the near future focused on readability (and possibility writability) of any APIs we use outside the computed artifacts

yes definitely, we're somewhat hamstrung at this exact moment by not having landed the mechanism to branch logic based on lantern vs. non-lantern which is immediate followup after this 👍

patrickhulce · 2018-03-20T20:12:21Z

alright thanks for the initial round of feedback folks! I think we're ready for round 2️⃣ 🥊 💨

patrickhulce · 2018-03-23T23:01:42Z

🏏 🏏

patrickhulce · 2018-03-23T23:01:51Z

🏓

patrickhulce · 2018-03-26T15:41:40Z

(cricket cricket)

(ping)

paulirish · 2018-03-26T22:27:48Z

lighthouse-core/gather/computed/metrics/lantern-metric.js

+    const optimisticEstimate = new Simulator(optimisticGraph, options).simulate();
+    const pessimisticEstimate = new Simulator(pessimisticGraph, options).simulate();
+
+    optimisticEstimate.timeInMs = this.getEstimateFromSimulation(


this bit still seems weird to me. here's how i'm reading it:

simulator, plz simulate with this graph.

simulation done here's my estimate and some nodeTiming internals

okay thx

but if this is the CI metric, i'm gonna do some extra work where I extract the lastLongTaskEndTime from the iternals and also take the lantern FMP number, and do some Math.max with those. and then i'm gonna ignore the provided estimate and instead use this one.

Is that right?

I'm thinking one alternative a sort of minimumEstimateTime(nodeTiming){ } method that's either provided to the simulator/simulate or something? Don't know if that makes sense though..

Are there any clients of simulate() now that don't override timeInMs with their own value?

I think this is a misunderstanding based on my laziness :) Simulator has no concept of a metric at all, and that's a good thing we'd like to keep; it just computes timings of things.

Metrics take these timings and return estimates, in the current case 1 optimistic, 1 pessimistic.

It just so happens that 2 of our 3 current metrics use time of the last network resource (what simulator outputs) as their estimate without any additional logic. Rather than add a mapSimulatorOutputToEstimateOutput I abused the existing return object and allow the metric to mutate it if it wants.

Given the amount of confusion this causes and the fact that we're about to be in a situation where only 2/5 metrics take simulator output without touching it, I think it makes sense to just go the mapSimulatorOutputToEstimateOutput route, WDYT? Does this make more sense?

@brendankenny since paul's OOO for awhile WDYT of the above? :)

brendankenny · 2018-03-29T00:57:28Z

lighthouse-cli/run.js

@@ -50,7 +50,7 @@ function parseChromeFlags(flags = '') {
 * Attempts to connect to an instance of Chrome with an open remote-debugging
 * port. If none is found, launches a debuggable instance.
 * @param {!LH.Flags} flags
- * @return {!Promise<!LH.LaunchedChrome>}
+ * @return {!Promise<LH.LaunchedChrome>}


any reason to half delete the ! here? :)

full 🗑 now!

brendankenny · 2018-03-29T01:01:19Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

+    graph.traverse(node => node.record && records.push(node.record));
+    const simulatorOptions = NetworkAnalysis.computeRTTAndServerResponseTime(records);
+    // TODO: use rtt/throughput from config.settings instead of defaults
+    delete simulatorOptions.rtt;


we should probably keep an eye on performance then, but hopefully properties on simulatorOptions aren't the bottleneck anyways... :)

brendankenny · 2018-03-29T01:21:32Z

lighthouse-core/gather/computed/metrics/lantern-metric.js

+    const optimisticEstimate = new Simulator(optimisticGraph, options).simulate();
+    const pessimisticEstimate = new Simulator(pessimisticGraph, options).simulate();
+
+    optimisticEstimate.timeInMs = this.getEstimateFromSimulation(


brendankenny · 2018-03-29T01:23:43Z

tsconfig.json

@@ -6,6 +6,7 @@
    "allowJs": true,
    "checkJs": true,
    "strict": true,
+    "allowSyntheticDefaultImports": true,


was this needed or did it turn out to be a red herring?

yeah unnecessary, removed

brendankenny · 2018-03-29T01:26:09Z

typings/gatherer.d.ts

+
+    namespace Simulation {
+      // HACK: TS treats 'import * as Foo' as namespace instead of a type, use typeof and prototype
+      export type GraphNode = typeof _Node['prototype'];


2.8 ended up adding a new helper that might(?) work here (and be somewhat less ugly :). export type GraphNode = InstanceType<typeof _Node>;

patrickhulce · 2018-03-30T18:44:52Z

hey folks would love to merge this today if possible, any more comments @paulirish @brendankenny ?

brendankenny · 2018-03-30T21:50:18Z

lighthouse-extension/test/extension-test.js

@@ -76,6 +76,7 @@ describe('Lighthouse chrome extension', function() {
    });

    if (lighthouseResult.exceptionDetails) {
+      console.error(lighthouseResult); // eslint-disable-line no-console


will you add a comment explaining why logging here?

brendankenny

LGTM!

patrickhulce requested review from brendankenny and paulirish as code owners March 14, 2018 23:32

patrickhulce commented Mar 14, 2018

View reviewed changes

devtools-bot added the waiting4reviewer label Mar 14, 2018

patrickhulce mentioned this pull request Mar 14, 2018

deps(browserify): bump browserify for async/await #4767

Merged

core(lantern): move metrics to computed artifacts

f7c0324

patrickhulce force-pushed the move_predictive_perf_artifact branch from 218ed30 to f7c0324 Compare March 15, 2018 16:30

This was referenced Mar 15, 2018

PROPOSAL: new structure for metrics + lantern #4676

Closed

core(metrics): add lantern speed index #4695

Merged

"es8 isn't a real thing" - paulirish

43e6ffe

patrickhulce mentioned this pull request Mar 19, 2018

tests(smokehouse): adjust byte efficiency CPU multiplier #4809

Merged

Merge branch 'master' into move_predictive_perf_artifact

5f34338

paulirish reviewed Mar 20, 2018

View reviewed changes

brendankenny reviewed Mar 20, 2018

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Mar 20, 2018

patrickhulce added 3 commits March 20, 2018 11:12

Merge branch 'master' into move_predictive_perf_artifact

17be96d

typecheck!

be7b1d3

move to lantern- prefix

d4d2b5f

devtools-bot added waiting4reviewer and removed waiting4committer labels Mar 20, 2018

patrickhulce added 3 commits March 20, 2018 13:05

Merge branch 'master' into move_predictive_perf_artifact

0be326d

remove tern

9d64fa9

cleanup d.ts file

6a75bf8

patrickhulce mentioned this pull request Mar 26, 2018

Move all metrics to computed artifacts #4872

Closed

paulirish requested changes Mar 26, 2018

View reviewed changes

brendankenny reviewed Mar 29, 2018

View reviewed changes

patrickhulce added 4 commits March 29, 2018 10:24

feedback

413ec5d

cleanup timeInMs rewrite business

ad61451

Merge branch 'master' into move_predictive_perf_artifact

4c8cdc4

s/TaskMultiplier/SlowdownMultiplier

d794222

paulirish approved these changes Mar 30, 2018

View reviewed changes

patrickhulce added 2 commits March 30, 2018 14:27

log extension test error

d8296eb

fix extension gulp

4694b1f

brendankenny reviewed Mar 30, 2018

View reviewed changes

last feedback

25e0650

brendankenny approved these changes Mar 30, 2018

View reviewed changes

patrickhulce merged commit 25913c7 into master Mar 30, 2018

patrickhulce deleted the move_predictive_perf_artifact branch March 30, 2018 22:16

paulirish removed the waiting4reviewer label Apr 30, 2018

		@@ -0,0 +1,100 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

	const gatherers = LighthouseRunner.getGathererList()
	.map(f => '../lighthouse-core/gather/gatherers/' + f.replace(/\.js$/, ''));

core(lantern): move metrics to computed artifacts #4766

core(lantern): move metrics to computed artifacts #4766

Conversation

patrickhulce commented Mar 14, 2018

Choose a reason for hiding this comment

patrickhulce commented Mar 19, 2018

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Mar 20, 2018

patrickhulce commented Mar 20, 2018

patrickhulce commented Mar 23, 2018

patrickhulce commented Mar 23, 2018

patrickhulce commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce Mar 20, 2018 •

edited

Loading